[flang][fir] Small clean-up in fir_DoConcurrentLoopOp's defintion
#146028
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-fir-hlfir Author: Kareem Ergawy (ergawy) ChangesRe-organizes the op definition a little bit and removes a method that does not add much value to the API. Full diff: https://github.com/llvm/llvm-project/pull/146028.diff 2 Files Affected:
diff --git a/flang/include/flang/Optimizer/Dialect/FIROps.td b/flang/include/flang/Optimizer/Dialect/FIROps.td
index 75cf020ec96c8..f304f0cf30ac5 100644
--- a/flang/include/flang/Optimizer/Dialect/FIROps.td
+++ b/flang/include/flang/Optimizer/Dialect/FIROps.td
@@ -3884,9 +3884,17 @@ def fir_DoConcurrentLoopOp : fir_Op<"do_concurrent.loop",
let hasVerifier = 1;
let extraClassDeclaration = [{
- unsigned getNumInductionVars() { return getLowerBound().size(); }
+ unsigned getNumInductionVars() {
+ return getLowerBound().size();
+ }
- unsigned getNumLocalOperands() { return getLocalVars().size(); }
+ unsigned getNumLocalOperands() {
+ return getLocalVars().size();
+ }
+
+ unsigned getNumReduceOperands() {
+ return getReduceVars().size();
+ }
mlir::Block::BlockArgListType getInductionVars() {
return getBody()->getArguments().slice(0, getNumInductionVars());
@@ -3906,16 +3914,6 @@ def fir_DoConcurrentLoopOp : fir_Op<"do_concurrent.loop",
/// Number of operands controlling the loop
unsigned getNumControlOperands() { return getLowerBound().size() * 3; }
- // Get Number of reduction operands
- unsigned getNumReduceOperands() {
- return getReduceVars().size();
- }
-
- mlir::Operation::operand_range getLocalOperands() {
- return getOperands()
- .slice(getNumControlOperands() + getNumReduceOperands(),
- getNumLocalOperands());
- }
}];
}
diff --git a/flang/lib/Optimizer/OpenMP/DoConcurrentConversion.cpp b/flang/lib/Optimizer/OpenMP/DoConcurrentConversion.cpp
index 28f6c8bf02813..709cf1d0938fa 100644
--- a/flang/lib/Optimizer/OpenMP/DoConcurrentConversion.cpp
+++ b/flang/lib/Optimizer/OpenMP/DoConcurrentConversion.cpp
@@ -314,9 +314,9 @@ class DoConcurrentConversion
// For `local` (and `local_init`) opernads, emit corresponding `private`
// clauses and attach these clauses to the workshare loop.
- if (!loop.getLocalOperands().empty())
+ if (!loop.getLocalVars().empty())
for (auto [op, sym, arg] : llvm::zip_equal(
- loop.getLocalOperands(),
+ loop.getLocalVars(),
loop.getLocalSymsAttr().getAsRange<mlir::SymbolRefAttr>(),
loop.getRegionLocalArgs())) {
auto localizer = mlir::SymbolTable::lookupNearestSymbolFrom<
|
ergawy
force-pushed
the
users/ergawy/convert_locality_specs_to_clauses_6
branch
from
5073e7d to
9021dbc
Compare
ergawy
force-pushed
the
users/ergawy/convert_locality_specs_to_clauses_7
branch
from
d9ca670 to
5f665c9
Compare
tblah
approved these changes
Jun 27, 2025
ergawy
force-pushed
the
users/ergawy/convert_locality_specs_to_clauses_6
branch
from
9021dbc to
6a7c71e
Compare
ergawy
force-pushed
the
users/ergawy/convert_locality_specs_to_clauses_7
branch
2 times, most recently
from
105ac7a to
3e2bdf5
Compare
ergawy
force-pushed
the
users/ergawy/convert_locality_specs_to_clauses_6
branch
from
926cb7c to
aa3984a
Compare
ergawy
force-pushed
the
users/ergawy/convert_locality_specs_to_clauses_7
branch
from
3e2bdf5 to
15ed831
Compare
ergawy
force-pushed
the
users/ergawy/convert_locality_specs_to_clauses_6
branch
from
aa3984a to
4db4b85
Compare
ergawy
force-pushed
the
users/ergawy/convert_locality_specs_to_clauses_7
branch
from
15ed831 to
53f9c0d
Compare
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Jul 10, 2025
…lled in OpenMP and OpenACC This PR proposes re-modelling `reduce` specifiers to match OpenMP and OpenACC. In particular, this PR includes the following: * A new `fir` op: `fir.delcare_reduction` which is identical to OpenMP's `omp.declare_reduction` op. * Updating the `reduce` clause on `fir.do_concurrent.loop` to use the new op. * Re-uses the `ReductionProcessor` component to emit reductions for `do concurrent` just like we do for OpenMP. To do this, the `ReductionProcessor` had to be refactored to be more generalized. * Upates mapping `do concurrent` to `fir.loop ... unordered` nests using the new reduction model. Unfortunately, this is a big PR that would be difficult to divide up in smaller parts because the bottom of the changes are the `fir` table-gen changes to `do concurrent`. However, doing these MLIR changes cascades to the other parts that have to be modified to not break things. This PR goes in the same direction we went for `private/local` speicifiers. Now the `do concurrent` and OpenMP (and OpenACC) dialects are modelled in essentially the same way which makes mapping between them more trivial, hopefully. PR stack: - llvm#145837 (this one) - llvm#146025 - llvm#146028 - llvm#146033
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Jul 10, 2025
With llvm#145837, the `ReductionProcessor` component is now used by both OpenMP and `do concurrent`. Therefore, this PR moves it to a shared location: `flang/Lower/Support`. PR stack: - llvm#145837 - llvm#146025 (this one) - llvm#146028 - llvm#146033
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Jul 10, 2025
Re-organizes the op definition a little bit and removes a method that does not add much value to the API. PR stack: - llvm#145837 - llvm#146025 - llvm#146028 (this one) - llvm#146033
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Jul 10, 2025
Now that we have changes introduced by llvm#145837, mapping reductions from `do concurrent` to OpenMP is almost trivial. This PR adds such mapping. PR stack: - llvm#145837 - llvm#146025 - llvm#146028 - llvm#146033 (this one)
ergawy
added a commit
that referenced
this pull request
Jul 11, 2025
…lled in OpenMP and OpenACC (#145837) This PR proposes re-modelling `reduce` specifiers to match OpenMP and OpenACC. In particular, this PR includes the following: * A new `fir` op: `fir.delcare_reduction` which is identical to OpenMP's `omp.declare_reduction` op. * Updating the `reduce` clause on `fir.do_concurrent.loop` to use the new op. * Re-uses the `ReductionProcessor` component to emit reductions for `do concurrent` just like we do for OpenMP. To do this, the `ReductionProcessor` had to be refactored to be more generalized. * Upates mapping `do concurrent` to `fir.loop ... unordered` nests using the new reduction model. Unfortunately, this is a big PR that would be difficult to divide up in smaller parts because the bottom of the changes are the `fir` table-gen changes to `do concurrent`. However, doing these MLIR changes cascades to the other parts that have to be modified to not break things. This PR goes in the same direction we went for `private/local` speicifiers. Now the `do concurrent` and OpenMP (and OpenACC) dialects are modelled in essentially the same way which makes mapping between them more trivial, hopefully. PR stack: - #145837 (this one) - #146025 - #146028 - #146033
ergawy
force-pushed
the
users/ergawy/convert_locality_specs_to_clauses_6
branch
from
4db4b85 to
e9eb77f
Compare
ergawy
force-pushed
the
users/ergawy/convert_locality_specs_to_clauses_7
branch
from
53f9c0d to
8e67de7
Compare
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jul 11, 2025
…ns are modelled in OpenMP and OpenACC (#145837) This PR proposes re-modelling `reduce` specifiers to match OpenMP and OpenACC. In particular, this PR includes the following: * A new `fir` op: `fir.delcare_reduction` which is identical to OpenMP's `omp.declare_reduction` op. * Updating the `reduce` clause on `fir.do_concurrent.loop` to use the new op. * Re-uses the `ReductionProcessor` component to emit reductions for `do concurrent` just like we do for OpenMP. To do this, the `ReductionProcessor` had to be refactored to be more generalized. * Upates mapping `do concurrent` to `fir.loop ... unordered` nests using the new reduction model. Unfortunately, this is a big PR that would be difficult to divide up in smaller parts because the bottom of the changes are the `fir` table-gen changes to `do concurrent`. However, doing these MLIR changes cascades to the other parts that have to be modified to not break things. This PR goes in the same direction we went for `private/local` speicifiers. Now the `do concurrent` and OpenMP (and OpenACC) dialects are modelled in essentially the same way which makes mapping between them more trivial, hopefully. PR stack: - llvm/llvm-project#145837 (this one) - llvm/llvm-project#146025 - llvm/llvm-project#146028 - llvm/llvm-project#146033
Base automatically changed from
users/ergawy/convert_locality_specs_to_clauses_6
to
main
July 11, 2025 05:42
ergawy
force-pushed
the
users/ergawy/convert_locality_specs_to_clauses_7
branch
from
8206c40 to
8e67de7
Compare
Re-organizes the op definition a little bit and removes a method that does not add much value to the API.
ergawy
force-pushed
the
users/ergawy/convert_locality_specs_to_clauses_7
branch
from
8e67de7 to
df63fea
Compare
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jul 11, 2025
… (#146025) With #145837, the `ReductionProcessor` component is now used by both OpenMP and `do concurrent`. Therefore, this PR moves it to a shared location: `flang/Lower/Support`. PR stack: - llvm/llvm-project#145837 - llvm/llvm-project#146025 (this one) - llvm/llvm-project#146028 - llvm/llvm-project#146033
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jul 11, 2025
…defintion (#146028) Re-organizes the op definition a little bit and removes a method that does not add much value to the API. PR stack: - llvm/llvm-project#145837 - llvm/llvm-project#146025 - llvm/llvm-project#146028 (this one) - llvm/llvm-project#146033
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jul 11, 2025
…` to OpenMP (#146033) Now that we have changes introduced by #145837, mapping reductions from `do concurrent` to OpenMP is almost trivial. This PR adds such mapping. PR stack: - llvm/llvm-project#145837 - llvm/llvm-project#146025 - llvm/llvm-project#146028 - llvm/llvm-project#146033 (this one)
|
Please include @khaki3 on future do concurrent work, thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Re-organizes the op definition a little bit and removes a method that does not add much value to the API.
PR stack:
reduceto match reductions are modelled in OpenMP and OpenACC #145837ReductionProcessortoLower/Support. #146025fir_DoConcurrentLoopOp's defintion #146028 (this one)do concurrent ... reduceto OpenMP #146033